vtxo: widen onchain spend tracking to onchain-kind vtxos - #1184
vtxo: widen onchain spend tracking to onchain-kind vtxos#1184bitcoin-coder-bob wants to merge 17 commits into
Conversation
Adds a vtxo_kind column (0 = offchain, 1 = onchain) so an on-chain Arkade UTXO (issue #1159) can be told apart from a batch leaf or offchain-tx output. Without it, such a vtxo misclassifies: empty commitment fields read as a note, and the sweeper would treat SpentBy as a checkpoint txid. - domain.Vtxo gains a Kind field; IsNote() keys off it so an on-chain vtxo with empty commitments is not a note. - sqlite + postgres migrations add the column (DEFAULT 0 backfills all history as offchain) and recreate vtxo_vw / intent_with_inputs_vw so it is visible. badger persists it via gob with no migration; old records decode to offchain. - UpsertVtxo and rowToVtxo carry Kind on both SQL backends. Chosen as an explicit enum over a bool so future on-chain sub-kinds need no further migration. Behaviour-preserving: nothing writes Onchain yet, so every existing row is offchain and all classifiers evaluate as before. The protective read-guards (sweeper/indexer filters) land with the on-chain write path, where they are testable against real on-chain rows. Part of #1159.
rowToVtxo on both SQL backends already carried Kind, but the round-replay converter (combinedRowToVtxo) and the marker converters built domain.Vtxo values without it, so a vtxo read through those paths came back as VtxoKindOffchain regardless of its stored kind. Any guard keyed on Kind downstream of round replay or marker preload would have been silently wrong. Note combinedRowToVtxo also drops Depth and MarkerIDs, which predates this work. Left alone here rather than widening the change, but it means round replay does not reconstruct the DAG fields either.
An on-chain Arkade UTXO has no batch expiry, so ExpiresAt is not meaningful for it and is left zero. Without this guard IsExpired would compare against the Unix epoch and report every on-chain vtxo as expired, which cascades through RequiresForfeit and the two spend-path checks in service.go and would make them permanently unspendable. Behaviour-preserving today: nothing writes VtxoKindOnchain yet, so every existing row is offchain and evaluates exactly as before.
An on-chain Arkade UTXO joins a batch as a boarding input, signed directly and never forfeited. IsNote already excludes the kind, which flipped RequiresForfeit to true for it, so guard it explicitly alongside IsNote and IsExpired.
Same up, down, re-apply sequence on both SQL backends, against a database the test owns on the shared test server.
Closes #1181. Part of #1159, follow-up to #1174 on top of #1161. #1174 tracks the onchain spends of unrolled vtxos, the only vtxos with an onchain output until now. With #1161 arkd also records vtxos held in on-chain Arkade UTXOs, Kind = Onchain, whose unilateral spends bypass arkd exactly as an unrolled vtxo's do. Every predicate the tracking keyed on unrolled now takes either shape: - Vtxo.HasOnchainOutput is the one Go predicate, unrolled or onchain kind, and IsOnchainSpent builds on it. - The five sql statements, mark, re-point, retract and the two candidate selectors, match (unrolled = true OR vtxo_kind = 1) on sqlite and postgres. - The badger selectors take both shapes and its mark guard uses the predicate. - applyOnchainSpends and the restore loop in restoreWatchingVtxos follow, the latter through the widened selectors. SelectSweepableUnrolledVtxos stays as it is: an onchain-kind UTXO has no checkpoint tx for the sweeper to resolve, and its ark_txid predicate already keeps onchain spends out. Tests, on all three backends with Kind = Onchain rows inserted directly so nothing waits for the write path: mark, re-point and retract work on an onchain-kind row, the selectors partition by kind as they do by unrolled, and an onchain-kind row never enters the sweepable set. The IsOnchainSpent table and the applyOnchainSpends unit test gain the kind cases. Removing the kind from the predicate fails the domain and application cases; the sql cases cannot pass on the old statements, which never matched a row that was not unrolled.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — widen onchain spend tracking to onchain-kind vtxos
This PR extends the onchain spend tracking and domain logic to cover VtxoKindOnchain vtxos (part of issue #1159), building on the VtxoKind discriminator introduced in #1161.
Domain changes are correct:
HasOnchainOutput()correctly captures both unrolled vtxos and onchain-kind vtxos. Using this helper consistently in place of rawUnrolledchecks keeps the two categories in sync.IsNote(): adding theKind != VtxoKindOnchainguard is necessary — an on-chain Arkade UTXO has no commitment txids, which would otherwise make it read as a note.IsExpired(): guarding on-chain kind is critical — a zeroExpiresAt(no batch expiry) would read as permanently expired, making the vtxo appear unspendable everywhere.RequiresForfeit(): on-chain kind vtxos join a batch as boarding inputs and are signed directly, never forfeited. The guard is correct.IsOnchainSpent(): updated to useHasOnchainOutput()— correct.
Application layer:
applyOnchainSpendsswitches toHasOnchainOutput()— correct.restoreWatchingVtxoslog message and comment updated — no logic change.
Test coverage is thorough: each modified method has a new test case for VtxoKindOnchain.
This is stacked on #1161 (VtxoKind discriminator). Flagging for human review as it touches VTXO domain logic and sweep/spend tracking. Looks correct. ✓
The VtxoKind doc comment carried an issue number, which belongs in the commit message and the PR description rather than in the source. Two comments in the down-migration test used a colon where a sentence reads better, one of them as a bare Up label whose Down counterpart was already written as prose. The test's support constants sat above the test. Moved to the bottom with the rest of the scaffolding, so the file opens on the test.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana nudge — 2026-09-10: This PR has been open for 3 days without a review. @Kukks is anyone looking at this?
…' into pr1184 # Conflicts: # internal/core/application/service.go # internal/core/domain/vtxo.go # internal/core/domain/vtxo_repo.go
A test helper named the PR that introduced the on-chain kind, which belongs in the commit message rather than the source, and it read as though the helper only made sense against that PR. It now says what the fixture is. Two comments used a colon where a conjunction reads better. Comments only.
…tions The audits that produced the earlier comment cleanups only ever looked at Go files, so the SQL comments in both backends kept an issue number and two colon sentences. Same wording in postgres and sqlite, since the pair should stay readable side by side.
…' into pr1184 # Conflicts: # internal/core/application/onchain_spend.go
|
This PR (vtxo: widen onchain spend tracking to onchain-kind vtxos) has been open for 3+ days without review. @bitcoin-coder-bob is anyone looking at this? |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Flagged for human review — protocol-critical VTXO path.
The change is logically correct and well-covered by tests. Key observations:
HasOnchainOutput()is a clean abstraction that correctly replaces the rawvtxo.Unrolledguard inapplyOnchainSpends,markOnchainSpentVtxo, andrestoreWatchingVtxos.IsOnchainSpent()andIsExpired()are correctly extended forVtxoKindOnchain.- The badger
GetUnrolledUnspentVtxosandGetOnchainSpentVtxosuseOrto add theVtxoKindOnchainbranch. Note: badgerholdOrat the top level performs two full scans — correctness is fine, but worth keeping in mind at scale. - The postgres
SelectOnchainSpentVtxosquery correctly extends theWHEREclause to(unrolled = true OR vtxo_kind = 1). - Test coverage is thorough across domain unit tests (
TestVtxo_*), repository integration tests (onchain_spend_repo_test.go), and application-level tests (onchain_spend_test.go).
No security issues found. Looks ready to merge after human sign-off on the protocol-layer semantics.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
This PR has been open for 6+ days without review. @bitcoin-coder-bob is anyone looking at this?
Closes #1181. Part of #1159, Milestone 5. Follow-up to #1174 on top of #1161.
Important
Stacked. Based on
feat/unrolled-vtxo-onchain-spend(#1174) withbob/onchain-arkade-vtxo-kind(#1161) merged in, so the diff shows #1161's commits through the merge until both land on master. The change itself is the last commit. Once #1161 and #1174 merge, this rebases onto master and the merge commit goes away.Context
#1174 tracks the onchain spends of unrolled vtxos, the only vtxos with an onchain output until now. Its candidate selectors, its apply path and its restore-on-restart loop are all scoped to
unrolled = true, andVtxo.IsOnchainSpentderives "spent onchain" from unrolled plus the absence of an in-Ark spend marker.With #1161 arkd also records vtxos held in on-chain Arkade UTXOs,
Kind = Onchain. Their unilateral spends bypass arkd exactly as an unrolled vtxo's do, and need the same detection, retraction and guards.What changed
Every predicate the tracking keyed on unrolled now takes either shape.
Vtxo.HasOnchainOutputis the one Go predicate, unrolled or onchain kind, andIsOnchainSpentbuilds on it.(unrolled = true OR vtxo_kind = 1). The literal is the value theadd_vtxo_kindmigration defines, and a comment above the first statement says so.applyOnchainSpendsfilters on the predicate, and the restore loop inrestoreWatchingVtxosfollows through the widened selectors.SelectSweepableUnrolledVtxosstays as it is: an onchain-kind UTXO has no checkpoint tx for the sweeper to resolve, and itsark_txidpredicate already keeps onchain spends out.Nothing here writes
Kind = Onchainrows. That is M3.Tests
On sqlite, postgres and badger, with onchain-kind rows inserted directly so nothing waits for the write path:
The
IsOnchainSpenttable and theapplyOnchainSpendsunit test gain the kind cases. Removing the kind from the predicate fails both; the sql cases cannot pass on the old statements, which never matched a row that was not unrolled.go build,go vetandmake lintclean.Decided on the issue
#1159 question 9 is decided (decision): cosigned outputs the client reveals are registered at cosign time as a pending on-chain kind and promoted to onchain kind at confirmation. A pending output has nothing on chain for a unilateral spend to take and its CSV cannot mature before confirmation, so
HasOnchainOutputstays on the confirmed kind and a pending row never enters the predicates here. Adding the pending enum value later is a migration-free change that does not touch this diff.